feat: Add `MsgLeaveGroup` to group module #10887

aleem1314 · 2022-01-05T15:40:19Z

Description

Closes: #9657

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

aaronc · 2022-01-05T16:36:23Z

The challenge we're going to run into here is how do we adjust the threshold in ThresholdDecisionPolicy. Seems like that's not resolved from #9657. Ideas @aleem1314 @blushi @AmauryM ? Maybe we choose to switch to a PercentageDecisionPolicy or there's a method on DecisionPolicy to handle &/or reject members leaving

blushi · 2022-01-06T08:34:47Z

The challenge we're going to run into here is how do we adjust the threshold in ThresholdDecisionPolicy. Seems like that's not resolved from #9657. Ideas @aleem1314 @blushi @AmauryM ? Maybe we choose to switch to a PercentageDecisionPolicy or there's a method on DecisionPolicy to handle &/or reject members leaving

This issue could actually already happen when a group admin removes group members (through UpdateGroupMembers) and it's not currently handled... So I believe this should be addressed as a separate issue. I think a method on DecisionPolicy to reject members leaving if that would result in a conflict with its threshold (ie group.TotalWeight < policy.Threshold) could be a quick solution until we move to more flexible decision policies.

blushi · 2022-01-07T11:31:28Z

@aaronc Actually I've just realized that currently, you cannot create a proposal if the policy threshold is greater than the total group weight, see

cosmos-sdk/x/group/keeper/msg_server.go

Line 416 in aaa61e3

err = policy.Validate(g)

and

cosmos-sdk/x/group/types.go

Lines 115 to 117 in aaa61e3

    
           if threshold.Cmp(totalWeight) > 0 { 
        
           	return sdkerrors.Wrap(errors.ErrInvalid, "policy threshold should not be greater than the total group weight") 
        
           }

So in this case, current proposals get invalidated (because group is updated) and new proposals can't be created.

But I agree this is not ideal from a user standpoint, this should error earlier on members updates rather than on proposal creation, thoughts?

proto/cosmos/group/v1beta1/tx.proto

codecov · 2022-01-21T16:05:48Z

Codecov Report

Merging #10887 (a4bfd0c) into master (489b399) will decrease coverage by 0.02%.
The diff coverage is 82.28%.

@@            Coverage Diff             @@
##           master   #10887      +/-   ##
==========================================
- Coverage   66.02%   66.00%   -0.03%     
==========================================
  Files         704      665      -39     
  Lines       71853    69032    -2821     
==========================================
- Hits        47442    45562    -1880     
+ Misses      21472    20799     -673     
+ Partials     2939     2671     -268

Impacted Files	Coverage Δ
client/flags/flags.go	`20.68% <ø> (+0.35%)`	⬆️
client/tx/factory.go	`26.08% <0.00%> (-0.12%)`	⬇️
x/auth/client/cli/tips.go	`0.00% <0.00%> (ø)`
x/group/codec.go	`43.47% <50.00%> (+0.29%)`	⬆️
x/group/msgs.go	`53.87% <52.63%> (-0.05%)`	⬇️
x/group/keeper/msg_server.go	`71.92% <62.26%> (-0.78%)`	⬇️
x/group/simulation/operations.go	`66.92% <66.66%> (+0.05%)`	⬆️
x/group/client/testutil/tx.go	`99.77% <99.43%> (-0.03%)`	⬇️
x/auth/client/testutil/suite.go	`97.50% <100.00%> (+0.17%)`	⬆️
orm/encoding/ormkv/seq.go
... and 42 more

lgtm-com · 2022-01-21T16:09:11Z

This pull request introduces 1 alert when merging d877e54 into eb01537 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2022-01-24T10:23:48Z

This pull request introduces 1 alert when merging 90306c1 into 6ea2049 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2022-01-24T13:25:59Z

This pull request introduces 1 alert when merging cce7b03 into 6ea2049 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

lgtm-com · 2022-01-24T15:43:55Z

This pull request introduces 1 alert when merging 8f9bfb6 into 96bc6a5 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

proto/cosmos/group/v1beta1/tx.proto

x/group/client/cli/tx.go

x/group/keeper/msg_server.go

Co-authored-by: Amaury <[email protected]>

…roup

amaury1093

LGTM

x/group/keeper/keeper_test.go

…roup

blushi

Pre-approving

x/group/spec/03_messages.md

Co-authored-by: Marie Gauthier <[email protected]>

## Description It seems like #10887 broke group simulations, fixing them here. --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

feat: add proto definitions

1a890f6

blushi mentioned this pull request Jan 13, 2022

Handle group member weight updates that break threshold decision policy #10945

Closed

4 tasks

aleem1314 added 7 commits January 20, 2022 18:56

feat: implement msg

212613a

WIP: msg server

bad6dc0

Merge branch 'master' into aleem/9657-msg-leave-group

e662c5f

WIP: implementing msg server

085fa8a

Merge branch 'master' into aleem/9657-msg-leave-group

dff0bdb

chore: run make proto-gen

c458296

feat: add cli

e470c66

github-actions bot added the C:CLI label Jan 21, 2022

Merge branch 'master' into aleem/9657-msg-leave-group

d877e54

aleem1314 requested a review from blushi January 21, 2022 15:40

aleem1314 commented Jan 21, 2022

View reviewed changes

proto/cosmos/group/v1beta1/tx.proto Outdated Show resolved Hide resolved

wip: integration tests

90306c1

aleem1314 added 3 commits January 24, 2022 18:01

feat: add cli tests

2f2f9df

chore: fix test

6c166cb

Merge branch 'master' into aleem/9657-msg-leave-group

cce7b03

feat: add simulation

8f9bfb6

github-actions bot added the C:Simulations label Jan 24, 2022

aleem1314 added 2 commits January 26, 2022 18:39

chore: cleanup

932dcf3

Merge branch 'master' into aleem/9657-msg-leave-group

70ce88c

blushi suggested changes Jan 31, 2022

View reviewed changes

proto/cosmos/group/v1beta1/tx.proto Outdated Show resolved Hide resolved

aleem1314 and others added 2 commits February 28, 2022 19:45

fix imports

9f6f30b

Merge branch 'master' into aleem/9657-msg-leave-group

4cee605

amaury1093 reviewed Feb 28, 2022

View reviewed changes

proto/cosmos/group/v1beta1/tx.proto Outdated Show resolved Hide resolved

x/group/client/cli/tx.go Outdated Show resolved Hide resolved

x/group/client/cli/tx.go Outdated Show resolved Hide resolved

x/group/keeper/msg_server.go Outdated Show resolved Hide resolved

aleem1314 and others added 6 commits March 2, 2022 13:39

Update x/group/client/cli/tx.go

f9b4c5a

Co-authored-by: Amaury <[email protected]>

make proto-gen

1988cbc

Merge branch 'aleem/9657-msg-leave-group' into aleem/9657-msg-leave-g…

0e64917

…roup

Merge branch 'master' into aleem/9657-msg-leave-group

277cd05

make proto-gen

ad889b7

fix tests

62e7672

amaury1093 approved these changes Mar 2, 2022

View reviewed changes

x/group/keeper/keeper_test.go Show resolved Hide resolved

aleem1314 and others added 6 commits March 3, 2022 18:11

Merge branch 'master' into aleem/9657-msg-leave-group

237b574

chore: add test

bef38db

fix test

413dbeb

review changes

7e61240

chore: fix cli test

2addb63

Merge branch 'master' into aleem/9657-msg-leave-group

d9eb60a

blushi reviewed Mar 4, 2022

View reviewed changes

x/group/keeper/keeper_test.go Outdated Show resolved Hide resolved

x/group/keeper/keeper_test.go Show resolved Hide resolved

aleem1314 added 4 commits March 4, 2022 15:16

review changes

e95daee

Merge branch 'aleem/9657-msg-leave-group' into aleem/9657-msg-leave-g…

997e4a4

…roup

fix cli test

0d23961

Merge branch 'master' into aleem/9657-msg-leave-group

bb29821

aleem1314 requested a review from blushi March 4, 2022 11:17

blushi approved these changes Mar 4, 2022

View reviewed changes

x/group/spec/03_messages.md Outdated Show resolved Hide resolved

Update x/group/spec/03_messages.md

ea82a8c

Co-authored-by: Marie Gauthier <[email protected]>

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 4, 2022

Merge branch 'master' into aleem/9657-msg-leave-group

a4bfd0c

mergify bot merged commit 9ef249c into master Mar 4, 2022

mergify bot deleted the aleem/9657-msg-leave-group branch March 4, 2022 14:03

blushi mentioned this pull request Mar 10, 2022

fix: Fix group simulations #11347

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `MsgLeaveGroup` to group module #10887

feat: Add `MsgLeaveGroup` to group module #10887

aleem1314 commented Jan 5, 2022 •

edited

Loading

aaronc commented Jan 5, 2022

blushi commented Jan 6, 2022 •

edited

Loading

blushi commented Jan 7, 2022

codecov bot commented Jan 21, 2022 •

edited

Loading

lgtm-com bot commented Jan 21, 2022

lgtm-com bot commented Jan 24, 2022

lgtm-com bot commented Jan 24, 2022

lgtm-com bot commented Jan 24, 2022

amaury1093 left a comment

blushi left a comment

feat: Add MsgLeaveGroup to group module #10887

feat: Add MsgLeaveGroup to group module #10887

Conversation

aleem1314 commented Jan 5, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

aaronc commented Jan 5, 2022

blushi commented Jan 6, 2022 • edited Loading

blushi commented Jan 7, 2022

codecov bot commented Jan 21, 2022 • edited Loading

Codecov Report

lgtm-com bot commented Jan 21, 2022

lgtm-com bot commented Jan 24, 2022

lgtm-com bot commented Jan 24, 2022

lgtm-com bot commented Jan 24, 2022

amaury1093 left a comment

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

feat: Add `MsgLeaveGroup` to group module #10887

feat: Add `MsgLeaveGroup` to group module #10887

aleem1314 commented Jan 5, 2022 •

edited

Loading

blushi commented Jan 6, 2022 •

edited

Loading

codecov bot commented Jan 21, 2022 •

edited

Loading